feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs by SuperFluffy · Pull Request #1209 · astriaorg/astria

SuperFluffy · 2024-06-26T14:13:16Z

Summary

Replaces the bare asset ID bytes by full IBC ICS20 denoms.

Background

Bare denom IDs in public APIs are difficult to understand. While IBC ICS20 denoms are prescribed to either contain strings of the form port/channel/denom or ibc/<sha256-hex>, due to protobuf-to-json mapping protobuf bytes are always encoded as base64. With #1181 we parse denoms into either of the two forms and can thus allow users to provide assets as protobuf strings which are then parsed by sequencer.

In addition, since denominations of the form port/channel/denom can always be converted to ibc/<sha256>, this allows for more elegant fetching of denom-related data from storage.

Changes

Replace protobuf message fields from bytes to string (mostly in transaction/action types):
- bytes asset_id -> string asset
- bytes fee_asset_id -> string fee_asset
- repeated bytes fee_asset_ids -> repeated string fee_assets (and rename AllowedFeeAssetIdsResponse to AllowedFeeAsssetsRersponse)
Update all affected checked astria-core types to parse the asset strings to astria_core::primitive::v1::asset::Denoms instead of [u8; 32] arrays.
Remove the astria_core::primitive::v1::asset::Id type
Remove all notion of a default native asset type from astria_core (all services and tests are now expected to have their own notion of asset types; either through config/genesis or by defining their tests accordingly)
Update all sequencer StateWriteExt and StateWriteRead types that read or write assets to disk:
- instead of writing asset::Ids (in practice [u8; 32]), they now write asset::IbcPrefixed
- make all trait methods generic over a parameter TAsset: Into<asset::IbcPrefixed> which allows fetching data using astria::Denoms, astria::TracePrefixed, or astria::IbcPrecixed
update all storage keys that used IDs to use ibc-prefixed denoms instead
provide snapshot tests for all such storage keys
update all astria-cli subcommands that construct actions with assets to have arguments --asset or --fee-asset (using "nria" as the default if not provided)
update astria-composer to have an config env var ASTRIA_COMPOSER_FEE_ASSET

Testing

All tests that involve fees were updated and pass again.

Breaking Changelist

This is a network breaking change because all transaction/action messages that took bytes now take strings

SuperFluffy · 2024-06-26T14:16:36Z

crates/astria-composer/local.env.example

 ASTRIA_COMPOSER_GRPC_ADDR="0.0.0.0:0"

+# The asset to use for paying for transactions submitted to sequencer.
+ASTRIA_COMPOSER_FEE_ASSET="nria"


Note that this sets the payment fee for all actions to nria - the same as was previously hard coded. This might not be exactly what we want to do in the future.

SuperFluffy · 2024-06-26T14:17:19Z

crates/astria-composer/src/executor/bundle_factory/mod.rs

-        seq_action: SequenceAction,
-    },
+    #[error(transparent)]
+    FinishedQueueFull(Box<FinishedQueueFull>),


This is a clippy-fix because the variant was too big.

crates/astria-composer/src/executor/bundle_factory/mod.rs

SuperFluffy · 2024-06-26T14:22:11Z

crates/astria-composer/src/executor/bundle_factory/tests.rs

@@ -1,19 +1,22 @@
-#[cfg(test)]


The structure of these modules was extremely strange and needed to be updated.

SuperFluffy · 2024-06-26T14:28:45Z

crates/astria-sequencer/src/asset/query.rs

        });
    };
-    let asset_id = hex::decode(asset_id)
+    let asset = <[u8; 32]>::from_hex(asset_id)


I am torn on this one. The request still has the form asset/denom/<hex>. So asset/denom/ibc/<hex> isn't allowed, and neither is asset/denom/port/channel/base.

...s/snapshots/astria_sequencer__accounts__state_ext__tests__storage_keys_have_not_changed.snap

crates/astria-sequencer/src/accounts/state_ext.rs

crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/mod.rs

crates/astria-cli/src/commands/sequencer.rs

noot · 2024-06-26T22:29:38Z

crates/astria-core/src/protocol/transaction/v1alpha1/action.rs

    pub data: Vec<u8>,
    /// asset to use for fee payment.
-    pub fee_asset_id: asset::Id,
+    pub fee_asset: asset::Denom,


we should enforce for all actions that if the denom trace is >32 bytes, it must be in ibc/ prefixed form. ie don't accept traces that are excessively long as it's a DOS vector

Do you mean we should enforce this when parsing string fee_asset -> asset::Denom? Technically the upper limit for protobuf payloads is 4MB and sequencer just rejects payloads >200KB, but I can add an additional limit on the type of course.

SuperFluffy · 2024-06-27T11:46:54Z

@noot I have changed all keys that in some form employ ibc-prefixed assets to use the hex encoded sha256 in e968da4, in a new module crate::storage_keys::hunks::Asset.

crates/astria-composer/src/executor/bundle_factory/tests.rs

noot

looks good, great change!

joroshiba

approval for infra components

* main: (27 commits) refactor(sequencer): fix prepare proposal metrics (#1211) refactor(bridge-withdrawer): move generated contract bindings to crate (#1237) fix(sequencer) fix wrong metric and remove unused metric (#1240) feat(sequencer): implement transaction fee query (#1196) chore(cli)!: remove unmaintained rollup subcommand (#1235) release(sequencer): 0.14.1 patch release (#1233) feat(sequencer-utils): generate example genesis state (#1224) feat(sequencer): implement abci query for bridge account info (#1189) feat(charts): bridge-withdrawer, smoke test, various chart improvements (#1141) chore(charts): update for new geth update (#1226) chore(chart)!: dusk-8 chart version updates (#1223) release(conductor): fix conductor release version (#1222) release: dusk-8 versions (#1219) fix(core): revert `From` ed25519_consensus types for crypto mod (#1220) Refactor(chart, sequencer): restructure sequencer chart, adjust configs (#1193) refactor(withdrawer): read from block subscription stream and get events on each block (#1207) feat(core): implement with verification key for address builder and crypto improvements (#1218) feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs (#1209) chore(chart): update evm chart for new prefix field (#1214) chore: bump penumbra deps (#1216) ...

SuperFluffy requested review from a team, joroshiba and noot as code owners June 26, 2024 14:13

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate composer pertaining to composer cd labels Jun 26, 2024

SuperFluffy marked this pull request as draft June 26, 2024 14:14

SuperFluffy force-pushed the superfluffy/no-ids branch from cab1204 to 476e4fa Compare June 26, 2024 14:14

SuperFluffy temporarily deployed to BUF June 26, 2024 14:15 — with GitHub Actions Inactive

SuperFluffy marked this pull request as ready for review June 26, 2024 14:15

SuperFluffy commented Jun 26, 2024

View reviewed changes

crates/astria-composer/src/executor/bundle_factory/mod.rs Outdated Show resolved Hide resolved

SuperFluffy mentioned this pull request Jun 26, 2024

Rework transaction size estimation in composer #1210

Closed

SuperFluffy commented Jun 26, 2024

View reviewed changes

SuperFluffy temporarily deployed to BUF June 26, 2024 14:26 — with GitHub Actions Inactive

SuperFluffy commented Jun 26, 2024

View reviewed changes

joroshiba reviewed Jun 26, 2024

View reviewed changes

...s/snapshots/astria_sequencer__accounts__state_ext__tests__storage_keys_have_not_changed.snap Show resolved Hide resolved

noot reviewed Jun 26, 2024

View reviewed changes

crates/astria-sequencer/src/accounts/state_ext.rs Outdated Show resolved Hide resolved

noot reviewed Jun 26, 2024

View reviewed changes

crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/mod.rs Outdated Show resolved Hide resolved

noot reviewed Jun 26, 2024

View reviewed changes

crates/astria-cli/src/commands/sequencer.rs Outdated Show resolved Hide resolved

noot reviewed Jun 26, 2024

View reviewed changes

SuperFluffy temporarily deployed to BUF June 27, 2024 11:42 — with GitHub Actions Inactive

SuperFluffy force-pushed the superfluffy/no-ids branch from 9374b33 to e968da4 Compare June 27, 2024 11:44

SuperFluffy temporarily deployed to BUF June 27, 2024 11:44 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF June 27, 2024 12:14 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF June 27, 2024 12:20 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF June 27, 2024 15:33 — with GitHub Actions Inactive

SuperFluffy requested a review from noot June 27, 2024 17:03

noot reviewed Jun 27, 2024

View reviewed changes

crates/astria-composer/src/executor/bundle_factory/tests.rs Outdated Show resolved Hide resolved

noot approved these changes Jun 27, 2024

View reviewed changes

joroshiba approved these changes Jun 27, 2024

View reviewed changes

SuperFluffy temporarily deployed to BUF June 27, 2024 18:25 — with GitHub Actions Inactive

SuperFluffy enabled auto-merge June 27, 2024 18:27

SuperFluffy added 9 commits June 27, 2024 20:29

refactor(proto)!: replace all asset ID bytes by ics20 strings

e7c1f33

remove all id methods from assets

0f77018

fix broken test

6dbe29c

use sha part, not ibc prefix, for asset keys

23f4344

remove ibc prefixed from submitted state: not necessary

a405b24

remove asset IBC prefixed from cli output

80d0bb5

fix composer estimation of max action size

01886e5

removed the composer snapshot test (no point of it)

b3329c8

bump chart

9df3bd7

SuperFluffy force-pushed the superfluffy/no-ids branch from f06975d to 9df3bd7 Compare June 27, 2024 18:30

SuperFluffy temporarily deployed to BUF June 27, 2024 18:30 — with GitHub Actions Inactive

SuperFluffy added this pull request to the merge queue Jun 27, 2024

Merged via the queue into main with commit f05e829 Jun 27, 2024

SuperFluffy deleted the superfluffy/no-ids branch June 27, 2024 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs#1209

feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs#1209
SuperFluffy merged 9 commits intomainfrom
superfluffy/no-ids

SuperFluffy commented Jun 26, 2024

Uh oh!

SuperFluffy Jun 26, 2024

Uh oh!

SuperFluffy Jun 26, 2024

Uh oh!

Uh oh!

SuperFluffy Jun 26, 2024

Uh oh!

SuperFluffy Jun 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noot Jun 26, 2024

Uh oh!

SuperFluffy Jun 27, 2024

Uh oh!

SuperFluffy commented Jun 27, 2024

Uh oh!

Uh oh!

noot left a comment

Uh oh!

joroshiba left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

SuperFluffy commented Jun 26, 2024

Summary

Background

Changes

Testing

Breaking Changelist

Uh oh!

SuperFluffy Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SuperFluffy Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noot Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy commented Jun 27, 2024

Uh oh!

Uh oh!

noot left a comment

Choose a reason for hiding this comment

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants